Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add tests to entities #1192

Merged
merged 13 commits into from
Oct 3, 2023
Merged

Add tests to entities #1192

merged 13 commits into from
Oct 3, 2023

Conversation

bonjourmauko
Copy link
Member

@bonjourmauko bonjourmauko commented Sep 26, 2023 •
edited
Loading

Technical changes

  • Add tests to entities.

@bonjourmauko bonjourmauko added meta:quality-assurance level:starter Suited for beginners and new contributors labels Sep 26, 2023
@bonjourmauko bonjourmauko requested a review from a team September 26, 2023 23:23
@coveralls
Copy link

coveralls commented Sep 26, 2023 •
edited by github-actions bot
Loading

Coverage Status

coverage: 73.501% (-0.6%) from 74.127% when pulling 696e00f on compose-entity into 23979ed on master.

MattiSG
MattiSG reviewed Sep 27, 2023
View reviewed changes
Copy link
Member

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting changes, thank you!

openfisca_core/entities/group_entity.py Outdated Show resolved Hide resolved
openfisca_core/entities/group_entity.py Outdated Show resolved Hide resolved
openfisca_core/entities/group_entity.py Outdated Show resolved Hide resolved
openfisca_core/entities/typing.py Outdated Show resolved Hide resolved
openfisca_core/entities/tests/test_role.py Outdated Show resolved Hide resolved
openfisca_core/entities/tests/test_group_entity.py Outdated Show resolved Hide resolved
openfisca_core/entities/tests/test_group_entity.py Outdated Show resolved Hide resolved
openfisca_core/entities/tests/test_entity.py Outdated Show resolved Hide resolved
@bonjourmauko
Copy link
Member Author

@benjello This PR opened a couple of questions that I pose here for documentation purposes:

  1. Why the reliance on roles/subroles indexes?
  2. Why do we need bidirectional navigation entity <--> role?
  3. Why do we need bidirectional navigation entity <--> TBS?
  4. How does / where does the magic happen with the members_role projector?
  5. Why defining entity.CHILD and not something like entity.roles.where(key="child")?

These are all breaking-changes, but refactoring each could greatly facilitate maintenance and new features.

@bonjourmauko
Copy link
Member Author

Is there anything missing for LTGM @benjello @MattiSG?

benjello
benjello approved these changes Oct 3, 2023
View reviewed changes
Copy link
Member

@benjello benjello left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions left to the author of the PR to adopt or not.

openfisca_core/entities/entity.py Outdated Show resolved Hide resolved
openfisca_core/entities/entity.py Outdated Show resolved Hide resolved
@bonjourmauko bonjourmauko merged commit a3c2f83 into master Oct 3, 2023
22 checks passed
@bonjourmauko bonjourmauko deleted the compose-entity branch October 3, 2023 23:32
@bonjourmauko bonjourmauko added this to the Improved testing milestone Sep 17, 2024
@bonjourmauko bonjourmauko changed the title Add tests to entities [3/17] Add tests to entities Sep 17, 2024
@bonjourmauko bonjourmauko added kind:test Adding missing tests or correcting existing tests and removed meta:quality-assurance labels Sep 30, 2024
@bonjourmauko bonjourmauko changed the title [3/17] Add tests to entities Add tests to entities Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjello benjello benjello approved these changes

@MattiSG MattiSG Awaiting requested review from MattiSG

Assignees
No one assigned
Labels
kind:test Adding missing tests or correcting existing tests level:starter Suited for beginners and new contributors
Projects
None yet
Milestone
Improved testing
Development

Successfully merging this pull request may close these issues.
4 participants
@bonjourmauko @coveralls @MattiSG @benjello